Skip to content

Add default-resource-class-name config option#798

Merged
moskyb merged 7 commits into
buildkite:mainfrom
gempesaw:dg/default-resource-class-name
Jan 9, 2026
Merged

Add default-resource-class-name config option#798
moskyb merged 7 commits into
buildkite:mainfrom
gempesaw:dg/default-resource-class-name

Conversation

@gempesaw
Copy link
Copy Markdown
Contributor

@gempesaw gempesaw commented Jan 7, 2026

Background

We really like using resource_classes from #668 to be able to vary CPU/memory requests based off a single agent template, but currently if a pipeline step doesn't specify a resource_class: foo, they don't get any resource requests at all. I was hoping perhaps specifying resources in pod-spec-patch would be the fallback/default in that case, but pod-spec-patch's resources override anything from resource_class:

applyResourceClass happens around 524:

// Handle resource_class tag
if err := w.applyResourceClass(podSpec, tags); err != nil {
return nil, err
}

and then later on the pod-spec-patch and plugin spec patches are applied on top:

// Allow podSpec to be overridden by the controller configuration and the k8s plugin
// Patch from the controller is applied first
if w.cfg.PodSpecPatch != nil {
patched, err := PatchPodSpec(podSpec, w.cfg.PodSpecPatch, w.cfg.DefaultCommandParams, inputs.k8sPlugin, w.cfg.AllowPodSpecPatchUnsafeCmdMod)

// If present, patch from the k8s plugin is applied second.
if inputs.k8sPlugin != nil && inputs.k8sPlugin.PodSpecPatch != nil {
patched, err := PatchPodSpec(podSpec, inputs.k8sPlugin.PodSpecPatch, w.cfg.DefaultCommandParams, inputs.k8sPlugin, w.cfg.AllowPodSpecPatchUnsafeCmdMod)

it'd be nice to have a default that applies when a step doesn't remember to specify a resource_class

Changes

Adds a new default-resource-class-name config option that references a named resource class from resource-classes. When a job doesn't have a resource_class agent tag, the default kicks in; jobs that explicitly set resource_class still take priority.

usage looks like:

resource-classes:
  small:
    resource:
      requests:
        cpu: "500m"
        memory: "512Mi"
  large:
    resource:
      requests:
        cpu: "2"
        memory: "4Gi"

default-resource-class-name: "small"

I also added resource-classes to the helm schema since it seemed to be missing?

Testing

  • Unit tests for applyResourceClass covering the various combinations (no tag/no default, tag specified, default fallback, tag overrides default, error cases)
  • Integration test that verifies the default resource class actually gets applied by checking cgroup limits

Allow specifying a default resource class that applies to jobs without
an explicit resource_class agent tag.
Jobs without a resource_class agent tag now fall back to the configured
default. Tag-specified classes still take priority.
@gempesaw gempesaw force-pushed the dg/default-resource-class-name branch from a63feb0 to 4f1baa4 Compare January 7, 2026 21:20
@gempesaw gempesaw marked this pull request as ready for review January 7, 2026 22:16
@gempesaw gempesaw requested a review from a team as a code owner January 7, 2026 22:16
Copy link
Copy Markdown
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks really good! i've been wanting to add this feature myself ever since we added resource classes, so thank you!

one note: it might be worth adding a validation in controller.ParseAndValidateconfig to ensure that the default resource class is included in the list of resource classes provided, as it would significantly shorten the loop on fixing errors like missspelling the default resource class or similar.

so we can fail fast back to the user when they do a syntax silly or what not
@gempesaw
Copy link
Copy Markdown
Contributor Author

gempesaw commented Jan 8, 2026

@moskyb

adding a validation in controller.ParseAndValidateconfig to ensure that the default resource class is included in the list of resource classes provided

oo yea that's a great idea, tyvm b6087f9

@gempesaw gempesaw requested a review from moskyb January 8, 2026 17:50
Copy link
Copy Markdown
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! thanks so much @gempesaw

@moskyb moskyb enabled auto-merge January 9, 2026 01:13
@moskyb moskyb merged commit 87c51da into buildkite:main Jan 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants